protocols/autonat: optionally use only global IPs#2618
protocols/autonat: optionally use only global IPs#2618elenaf9 merged 21 commits intolibp2p:masterfrom
Conversation
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
This looks good to me overall. Just two comments:
| let status_text = "no dial-request over relayed connections".to_string(); | ||
| (status_text, ResponseError::DialError) | ||
| let status_text = "refusing to dial peer with blocked observed address".to_string(); | ||
| (status_text, ResponseError::DialRefused) |
There was a problem hiding this comment.
Note: the go implementation returns a ResponseError::DialError here (but with the same status text). I decided to change it here because imo a DialRefused makes more sense. A client should not flip their status to private or reduce the confidence in their public status just because they accidentally picked a server that rejects them because of the observed address (though if the client also enabled only_global_ips this should not happen in practice).
Edit: should it be part of the spec to list the cases in which a server returns DialError?
There was a problem hiding this comment.
Edit: should it be part of the spec to list the cases in which a server returns DialError?
Yes. I think this is worth consolidating on. Mind driving it on libp2p/specs?
There was a problem hiding this comment.
Also added 7035422, which does the same change for the case that filter_valid_addrs filters out all addresses send from the client.
| let status_text = "no dial-request over relayed connections".to_string(); | ||
| (status_text, ResponseError::DialError) | ||
| let status_text = "refusing to dial peer with blocked observed address".to_string(); | ||
| (status_text, ResponseError::DialRefused) |
There was a problem hiding this comment.
Edit: should it be part of the spec to list the cases in which a server returns DialError?
Yes. I think this is worth consolidating on. Mind driving it on libp2p/specs?
Co-authored-by: Max Inden <mail@max-inden.de>
We only add an observed addresses for a peer to Behaviour.connected if the address is not relayed. Hence there is no need to check the address again in `filter_valid_addrs`.
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Latest changes look good to me. Feel free to merge once ready.
Optionally only perform dial-backs on peers that are observed at a global ip-address. This is relevant when multiple peers are in the same local network, in which case a peer could incorrectly assume themself to be public because a peer in the same local network was able to dial them. Thus servers should reject dial-back requests from clients with a non-global IP address, and at the same time clients should only pick connected peers as servers if they are global. Behind a config flag (enabled by default) to also allow use-cases where AutoNAT is needed within a private network.
Fix AutoNAT examples that became outdated due to libp2p#2959 and libp2p#2618.
3 participants
Description
Optionally only perform dial-backs on peers that are observed at a global ip-address.
This is relevant when multiple peers are in the same local network, in which case a peer could incorrectly assume themself to be public because a peer in the same local network was able to dial them. Thus servers should reject dial-back requests from clients with a non-global IP address, and at the same time clients should only pick connected peers as servers if they are global.
Behind a config flag (enabled by default) to also allow use-cases where AutoNAT is needed within a private network.
Continuation/ alternative to #2526 (cc @hamamo). Opened as a draft for now because of the open questions below.
Links to any relevant issues
Fixes #2514.
Open Questions
The
is_globalcheck for Ipv4 and Ipv6 addresses currently implements the same logic asstd::net::Ipv4Addr::is_global,std::net::Ipv6Addr::is_global, which are at the time of writing unstable behind theipflag.(See rust-lang/rust#27709 and rust-lang/rust#85604.)
For Ipv4 most of the internally called methods are stable, apart from the checks for
is_shared,is_reservedandis_benchmarking. Could we just remove those checks? In case ofis_sharedthe peer is still eligible for a dial-back and the other two cases should never occur in practice, no? In general, would it be enough to just check foris_private,is_loopbackandis_link_local? I am not sure, but since we only use this check for observed addresses, can any of the other cases even happen?For Ipv6 there is still some ongoing discussion around the
is_globalcheck, because is currently checks for the (global) scope of the address rather than global reachability (See rust-lang/rust#86634 and rust-lang/rust#85604). There are ongoing efforts to fix this (and eventually stabilize allipmethods), but they seem to be inactive since last year. Is this something that we could / should help push to completion?Change checklist